🤖 Kimi Code Review

Review Summary

This PR performs a significant refactoring by extracting common test types into a shared module (common.rs) and eliminating duplication across test files. The changes are well-structured and improve maintainability.

Issues Found

1. Potential panic in From<BlockBody> conversion (common.rs:185-192)

   Self {
       attestations: VariableList::new(attestations).expect("too many attestations"),
   }

   Using expect here could panic if attestations exceed the limit. Consider proper error handling.

2. Incomplete BitList initialization (common.rs:143-144)

   justified_slots: BitList::with_capacity(0).unwrap(),
   justifications_validators: BitList::with_capacity(0).unwrap(),

   These are initialized with capacity 0 but the actual data from TestState (justified_slots, justifications_validators) is ignored. This appears to be a bug - the data should be converted to BitList properly.

3. Missing validation in pubkey deserialization (common.rs:300-310)
   The deser_pubkey_hex function strips the "0x" prefix but doesn't validate the remaining string contains only hex characters before attempting decode.

4. Inconsistent naming - TestState in common.rs vs direct usage in other files. Consider renaming to avoid confusion with domain types.

Positive Aspects

• Excellent code deduplication - removes ~400 lines of duplicated types
• Proper module organization with clear separation of concerns
• Maintains backward compatibility through From trait implementations
• Good use of Rust's module system with super::common imports

Recommendations

1. Fix the BitList initialization issue by properly converting the input data
2. Replace expect calls with proper error handling
3. Add input validation in deser_pubkey_hex
4. Consider adding unit tests for the conversion functions

Automated review by Kimi (Moonshot AI) · custom prompt

🤖 Codex Code Review

Findings

1. Potentially disabling all integration tests in this crate
   crates/blockchain/Cargo.toml:9 adds autotests = false. That disables Cargo’s automatic discovery of crates/blockchain/tests/*.rs unless explicit [[test]] entries are added. I don’t see new [[test]] stanzas in this diff, so these spec tests may stop running in CI. If the intent is to control test discovery, add explicit [[test]] entries or drop autotests = false.

2. Fixture data for justification bits is silently ignored
   crates/blockchain/tests/common.rs:74-116 keeps the previous behavior of constructing justified_slots and justifications_validators as empty BitList and ignoring the fixture data (justifiedSlots, justificationsValidators). That’s likely OK for current tests, but it means the tests won’t catch regressions involving those fields. If the fixtures include meaningful data, consider populating those bitlists from the containers. This is especially relevant for justification/finalization logic.

Notes

• The refactor to tests/common.rs looks clean and improves maintainability. No security-sensitive logic is touched; changes are confined to test helpers and types.

Residual risks / test gaps

• If autotests = false is left as-is without explicit [[test]], these consensus/sig spec tests won’t run at all, reducing coverage around fork choice and signature verification.

If you want, I can propose the minimal [[test]] entries and/or a small helper to map fixture bitlists into BitList to increase coverage.

Automated review by OpenAI Codex · custom prompt

Greptile Summary

Successfully extracted 14 duplicated test type definitions from types.rs and signature_types.rs into a new shared common.rs module, eliminating 245 lines of code duplication. The refactoring includes all shared types (Container, Config, Checkpoint, BlockHeader, Validator, TestState, Block, BlockBody, attestation types, TestInfo, and deser_pubkey_hex helper), with proper From trait implementations preserved.

Key Changes:

• Created tests/common.rs with #![allow(dead_code)] attribute containing all shared types
• Updated types.rs to retain only fork-choice-specific types (ForkChoiceTestVector, ForkChoiceTest, ForkChoiceStep, etc.)
• Updated signature_types.rs to retain only signature-specific types (SSZ types, VerifySignaturesTestVector, ProposerSignature, etc.)
• Added mod common; declarations to both test runner files
• Added autotests = false to Cargo.toml to prevent module files from being discovered as standalone tests

Minor Normalization:
The From<BlockBody> implementation was standardized to use VariableList::new() (previously types.rs used this form while signature_types.rs used .collect::<Vec<_>>().try_into()). Both approaches are functionally equivalent.

No logical changes to test behavior. All existing tests should continue to pass.

Confidence Score: 5/5

• This PR is safe to merge with no risk - it's a pure code deduplication refactoring with no logical changes
• This is a straightforward code deduplication refactoring that extracts identical type definitions into a shared module. The changes are mechanical, preserve all functionality, and introduce no new logic. The standardization of the From<BlockBody> implementation uses equivalent approaches. The autotests = false setting is the correct way to prevent test helper modules from being treated as standalone tests.
• No files require special attention

_{Last reviewed commit: 806831e}

🤖 Claude Code Review

Now I have a complete picture of the PR. Let me write the review.

PR Review: Extract shared test types to common module

Summary: Clean refactoring with no logic changes. The 245-line reduction is accurate, and the module boundary design is correct. A few items worth noting:

autotests = false is safe

The existing bot reviews flagged missing [[test]] entries, but this is incorrect. The Cargo.toml already has explicit [[test]] stanzas for both forkchoice_spectests and signature_spectests (with harness = false and required-features correctly set). The autotests = false flag is exactly right here — without it, Cargo would try to compile common.rs, types.rs, and signature_types.rs as standalone test binaries, which would fail since they're helper modules, not entry points.

justified_slots / justifications_validators silently zeroed — pre-existing gap

common.rs:171-174:

justified_slots: BitList::with_capacity(0).unwrap(),
justifications_validators: BitList::with_capacity(0).unwrap(),

Both fields are initialized empty, ignoring the justifiedSlots: Container<u64> and justificationsValidators: Container<bool> values from the fixture's TestState. This is identical to the original behavior in both types.rs and signature_types.rs, so it's faithfully preserved — but it means fork choice spec tests never exercise initial justification state from fixtures. Worth a follow-up issue if fixtures ever encode meaningful justification bit data at the anchor state.

Validator field visibility narrowed

In the old signature_types.rs, Validator.index and Validator.pubkey were pub. In the new common.rs:116-120, they are private:

pub struct Validator {
    index: u64,          // was pub in signature_types.rs
    pubkey: ValidatorPubkeyBytes,  // was pub in signature_types.rs
}

This is fine in practice — nobody outside common.rs accesses these fields directly (the From impl lives in the same module), and the old types.rs also had them private. But if a future test needs to construct or inspect a Validator directly, the private fields will require touching common.rs. Minor, but worth being aware of.

deser_pubkey_hex bot review is a false alarm

common.rs:323:

hex::decode(value.strip_prefix("0x").unwrap_or(&value))
    .map_err(|_| D::Error::custom("ValidatorPubkey value is not valid hex"))?

hex::decode already validates that the input contains only valid hex characters before attempting decoding. No additional pre-validation is needed.

VariableList::new(attestations).expect("too many attestations") — pre-existing

common.rs:221. The panic message is appropriate for the failure mode. Pre-existing behavior from types.rs; no regression here.

Overall

The change is correct and achieves its goal cleanly. No behavior is altered — only duplication removed. The module structure (mod common; in each test binary, #![allow(dead_code)] at file scope) is the right approach for shared test helpers in Rust. Approve with no blocking concerns.

Automated review by Claude (Anthropic) · custom prompt